Skip to content

Buf proto update — v1.30.1 read-path UUID fix (Phases 19–22) - #39

Merged
onokonem merged 41 commits into
mainfrom
buf-proto-update-3
Jul 8, 2026
Merged

Buf proto update — v1.30.1 read-path UUID fix (Phases 19–22)#39
onokonem merged 41 commits into
mainfrom
buf-proto-update-3

Conversation

@onokonem

@onokonem onokonem commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Branch buf-proto-update-3 — protocol modernization increments shipping Phases 19–22 (49 commits ahead of main).

Latest increment — Phase 22: Wire the v1alpha1 DownloadManifestAndBlobs handler on *api to the existing Phase 18 UUID-resolution machinery on *commitServiceHandler (commitMap → resolveForeignCommitID → probeCommitID via commitUUIDInverse), so a 32-char buf-issued UUID carried in the request reference is resolved to its 40-char git SHA before being passed to GetFiles/GetTree. Re-runs the Phase 21 TestGenerateWithPinnedBufLock/v1.30.1 e2e regression gate to confirm the v1 protocol works end-to-end.

Goal (Phase 22, from ROADMAP): Fix the v1.30.1 v1alpha1 read-path UUID handling and verify the v1 protocol works.

Status: Phase 22 verified 4/5 must-haves; PR-22-4 live e2e blocked by a provider-transport TLS issue to raw.githubusercontent.com (handler-level fix independently proven by unit tests + live log). Approved on that evidence — see Verification section.

Changes

Phase 22 — Plan 22-01: v1alpha1 UUID→SHA resolution on read path

Thin CommitResolver interface on *api bridging v1alpha1 handlers to the Phase 18 commit-resolution ladder; no reimplementation.

Key files:

  • created: internal/connect/blobs_test.go (3 tests: warm-cache UUID resolution, cold-cache probe fallback, resolution-error no-HEAD-fallback)
  • modified: internal/connect/blobs.go (UUID-resolution branch before GetFiles)
  • modified: internal/connect/api.go (CommitResolver interface + field + NewWithConfig assignment)
  • modified: internal/connect/commits.go (resolveCommitForRead wrapper: commitMap → resolveForeignCommitID → probeCommitID)

Earlier increments on this branch (already executed/verified in their phases):

  • Phase 21 — TestGenerateWithPinnedBufLock e2e matrix + buf.gen.yaml plugin: (not remote:) fix
  • Phase 20 — parseResourceRefName reads Name.ref at field 4 (not field 3); default-branch-name carve-out in GetMeta (github + bitbucket)
  • Phase 19 — e2e tests for ref-honoring in buf.yaml deps

Requirements Addressed

  • PR-22-1 — v1alpha1 UUID resolved to SHA before provider.GetFiles
  • PR-22-2 — commitMap miss reaches probeCommitID (Phase 18 path)
  • PR-22-3 — unresolvable UUID returns Connect error, no HEAD fallback, no wrong content
  • PR-22-4TestGenerateWithPinnedBufLock/v1.30.1 passes live (⚠ env-blocked, see Verification)
  • PR-22-5 — Phase 18 helpers (commitUUIDInverse/isUUID/isSHA/probeCommitID) unchanged

Verification

  • Automated: PR-22-1/2/3/5 unit tests GREEN (run by verifier via direct go test); Phase 18 guard suite intact
  • go build ./..., go vet ./..., full go test ./... clean on merged tree
  • PR-22-4 live e2e (EASYP_GH_TOKEN=… go test ./e2e/ -run 'TestGenerateWithPinnedBufLock/v1.30.1') — blocked: the proxy server's Go HTTP client TLS-times-out to raw.githubusercontent.com (~11s) while system curl to the same URL returns 200 in 0.25s. UUID resolution fires correctly (live log shows SHA 99f54e6513… resolved and passed to multisource.GetFiles); the 500 is the provider's downstream file fetch. Root cause is provider-transport / OS-level process networking, outside Phase 22 scope. Tracked in 22-HUMAN-UAT.md.
  • ⚠ Code review (22-REVIEW.md): 1 critical (CR-01 stale digest in registerResolvedAlias when a probe resolves a module to a different commit) + 6 warnings — assessed by the verifier as pre-existing Phase 18 behavior / multi-module concerns outside this phase's tested topology; candidates for a future hardening phase.

Key Decisions

  • Interface named CommitResolver with unexported method resolveCommitForRead — keeps it same-package-only (both *api and *commitServiceHandler live in package connect) and matches Phase 18 ladder terminology.
  • Nil-guard at call site (isUUID(ref) && a.commitResolver != nil) as defense-in-depth, even though NewWithConfig always sets the pointer.
  • countingProvider wrapper defined in blobs_test.go (not by modifying mockProvider) to assert zero GetFiles calls in the resolution-error test.

🤖 Generated with Claude Code

onokonem added a commit that referenced this pull request Jul 8, 2026
onokonem and others added 29 commits July 8, 2026 16:53
Phase 19 closes the gap left by Phase 18: the proxy now honors
Name.ref end-to-end (commits 4fc6f28, 372bb15, 1d5fe33, bbaac3f),
but no e2e test exercises the real-server path with a real buf CLI
hitting a real GitHub API. The phase 19 plan formalizes the
in-progress drafts (e2e/ref_test.go + e2e/testutil/server.go) into
an executable 2-task plan.

Plan 19-01:
- Task 1: adopt RunBufModUpdateWithRef / RunBufDepUpdateWithRef /
  runBufUpdate in e2e/testutil/server.go; confirm testutil unit
  tests still pass.
- Task 2: adopt TestRefRespected_ModUpdate_DiffersFromHead /
  TestRefRespected_ModUpdate_MatchesUpstreamSHA /
  TestRefRespected_DepUpdate_DiffersFromHead in e2e/ref_test.go;
  verify compile + list + skip-clean + commit both files.

ROADMAP.md updated: Phase 19 goal, 3 success criteria, 1 plan link.

Co-Authored-By: Claude <noreply@anthropic.com>
Adds three e2e tests to verify the proxy honors the Name.ref field
in buf.yaml dependencies end-to-end via a real buf CLI + real GitHub API:

- TestRefRespected_ModUpdate_DiffersFromHead: matrix over all cached
  buf versions, asserts the no-ref and ref-pinned buf.lock files pin
  different commits.
- TestRefRespected_ModUpdate_MatchesUpstreamSHA: v1.69.0 only, uses
  git ls-remote to fetch the upstream SHA at the pinned ref, derives
  the expected UUID via the test-side commitUUIDForTest mirror, and
  asserts the lock matches.
- TestRefRespected_DepUpdate_DiffersFromHead: v1.69.0 only, same diff
  assertion via the modern "buf dep update" subcommand.

The testutil helper runBufUpdate is the shared implementation behind
the four public Run*Update helpers; it writes a buf.yaml whose dep
string is "127.0.0.1:<port>/googleapis/googleapis[:<ref>]" and reads
the resulting buf.lock back as raw bytes for the test to inspect.

All three tests skip cleanly (t.Skipf) when EASYP_GH_TOKEN /
EASYP_GITHUB_TOKEN is unset, matching the existing testutil pattern.

Co-Authored-By: Claude <noreply@anthropic.com>
Both Phase 19 e2e regressions trace to a single root cause: Phase 18's
parseResourceRefName reads proto field 3 from the Name.child oneof as
ref, but the buf v1beta1/v1 Name proto defines label_name=3 and ref=4.
One-line fix at commits_helpers.go:196; test mirror update at
commits_helpers_test.go:379; new regression-guard test for label_name
not being read as ref.

Co-Authored-By: Claude <noreply@anthropic.com>
- 20-RESEARCH.md: single root cause verified — parseResourceRefName reads
  field 3 (label_name) instead of field 4 (ref) per buf v1beta1/v1 proto
- 20-VALIDATION.md: per-task test map for the one-line production fix,
  the matching test update, and a new regression-guard test

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
… 3 (label_name)

The buf v1beta1/v1 Name.child oneof defines label_name=3 and ref=4.
The previous code read field 3 (label_name) and labelled it as ref,
which caused two Phase 19 regressions:

- v1.30.1 no-ref requests: buf CLI sends label_name="main" with no
  ref at field 4; the proxy misread label_name as ref="main" and
  called repos.GetCommit("main"), which GitHub rejects with 422.

- v1.69.0 ref-pinned requests: buf CLI sends ref=<tag> at field 4
  and (typically) label_name="main" at field 3; the proxy read
  label_name as ref when it was sent, or empty ref when not sent,
  and the providers returned HEAD instead of the SHA at the ref.

Reading field 4 binds the parsed ref to the buf client's actual
ref field. With ref="" the providers take the HEAD fast path
(no upstream call). With ref=<tag> the providers call
repos.GetCommit(ref) to resolve the SHA.

Co-Authored-By: Claude <noreply@anthropic.com>
… field 4

The existing test built a Name with ref at field 3 to match the
buggy production code. After the field-4 fix in commits_helpers.go,
the test must also write the ref at field 4 or it would fail with
"parseResourceRefName returned nil". This is the lockstep test
update — both must move together in the same change set.

The comment is updated to name field 4 and the Name.child oneof
shape, so a future reader sees the proto contract directly in the
test file.

TestParseResourceRefName_NoRef is unchanged: it builds owner+module
only (no ref field at all) and is unaffected by the field-number
fix.

Co-Authored-By: Claude <noreply@anthropic.com>
…n guard

The v1.30.1 case caught by Phase 19 e2e tests: the buf CLI sends
label_name="main" at proto field 3 (the default label name) with
no ref at field 4. The pre-fix code misread label_name as ref,
then called GetMeta("main") and hit a GitHub 422 on
repos.GetCommit("main").

This test pins the contract: field 3 is label_name, field 4 is
ref, and parseResourceRefName must read the latter (and ignore
the former). A regression in either direction — reading field 3
as ref, or refusing to parse a Name with label_name at field 3 —
is caught here. This is the test that would have caught the
original bug if it had existed in Phase 18.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…lpha1 bug

Phase 20 RESEARCH.md claimed v1.30.1 uses v1beta1.CommitService/
GetCommits. The live test (with EASYP_GH_TOKEN) shows v1.30.1
actually uses v1alpha1.ResolveService/GetModulePins and sends
ModuleReference.reference="main" directly through the generated
proto. The field-3/field-4 fix is correct for the v1.69.0 case
(TestRefRespected_ModUpdate_MatchesUpstreamSHA passes with the
correct SHA) but does not address v1.30.1's v1alpha1 path.

Recommend opening Phase 21 to apply the provider-level carve-out
in github/getrepo.go:48-58 and bitbucket/getrepo.go:43-53 (the
"defensive update" the research called optional but is actually
load-bearing for v1.30.1).

Co-Authored-By: Claude <noreply@anthropic.com>
The v1.30.1 v1alpha1 path (ResolveService/GetModulePins) sends
ModuleReference.reference="main" (the default label name) even
when no explicit ref is set. The proxy at modulepins.go:46 then
calls GetMeta(ctx, owner, repo, "main"), which routed through
repos.GetCommit(ctx, owner, repo, "main", nil) — GitHub's
/commits/main endpoint expects a SHA and rejects branch names
with 422 "No commit found for SHA: main".

The carve-out: when commit equals meta.DefaultBranch, return the
HEAD already in meta.Commit (set by getRepo via repos.GetBranch)
without a second round-trip. This restores the pre-Phase-18
behavior for default-branch-name inputs.

The carve-out is the smallest possible change: 1 new branch (with
an empty body, since meta.Commit is already correct), 1 comment
block above the if statement. The SHA fast path (Phase 17) and
the ref-resolution path (Phase 18) are unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>
The v1.30.1 v1alpha1 path (ResolveService/GetModulePins) sends
ModuleReference.reference="main" (the default label name) even
when no explicit ref is set. The proxy at modulepins.go:46 then
calls getMeta(ctx, "main"), which routed through c.getCommit
(Bitbucket's /commits/main endpoint) — the endpoint expects a
SHA and rejects branch names with 404.

The carve-out: when commit equals meta.DefaultBranch, return the
HEAD already in meta.Commit (set by getRepo from
repo.LatestCommit) without a second round-trip. This restores the
pre-Phase-18 behavior for default-branch-name inputs.

The carve-out is the smallest possible change: 1 new branch (with
an empty body, since meta.Commit is already correct), 1 comment
block above the if statement. The SHA fast path (Phase 17) and
the ref-resolution path (Phase 18) are unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>
The v1.30.1 v1alpha1 case caught by Phase 19 e2e tests: the buf
CLI sends ModuleReference.reference="main" (the default label
name) via v1alpha1.ResolveService/GetModulePins. The proxy at
modulepins.go:46 then called GetMeta(ctx, owner, repo, "main"),
which routed through repos.GetCommit(ctx, owner, repo, "main",
nil) — GitHub's /commits/main endpoint expects a SHA, not a
branch name, and rejects the request with 422.

This test pins the post-Phase-20-02 contract: when commit equals
meta.DefaultBranch, GetMeta returns HEAD from getRepo and does NOT
call repos.GetCommit. The WithGetCommitError trap catches a
regression that re-routes the default branch name through the
ref-resolution API (any unexpected GetCommit call returns an
error and fails the test), and the GetCommitCallCount assertion
provides a deterministic count check.

This is the test that would have caught the v1.30.1 bug if it
had existed in Phase 18.

Co-Authored-By: Claude <noreply@anthropic.com>
…uard

The v1.30.1 v1alpha1 case caught by Phase 19 e2e tests: the buf
CLI sends ModuleReference.reference="main" (the default label
name) via v1alpha1.ResolveService/GetModulePins. The proxy at
modulepins.go:46 then called getMeta(ctx, "main"), which routed
through c.getCommit (Bitbucket's /commits/main endpoint) — the
endpoint expects a SHA, not a branch name, and rejects the
request with 404.

This test pins the post-Phase-20-02 contract: when commit equals
meta.DefaultBranch, getMeta returns HEAD from getRepo (via
/branches/default) and does NOT call /commits/main. The
t.Errorf on the /commits/main path catches a regression that
re-routes the default branch name through the ref-resolution
API (any call to /commits/main fails the test), and the
commitsHits counter provides a deterministic count check.

This is the test that would have caught the v1.30.1 bug if it
had existed in Phase 18.

Co-Authored-By: Claude <noreply@anthropic.com>
…ster", "develop", "trunk")

The v1.30.1 v1alpha1 path (ResolveService/GetModulePins) sends
ModuleReference.reference="main" (the buf default label name) even
when the repo's default branch is NOT "main" — googleapis/googleapis
has default="master" but the v1.30.1 client sends "main".

The Phase 20-02 first cut used , which
matches when the repo's default IS "main" but fails when the default
is something else (like googleapis's "master"). The live e2e test
exposed this: the carve-out didn't trigger and the proxy still hit
GitHub's /commits/main endpoint with 422.

The fix: extend the carve-out to a small set of conventional default
branch/label names ("main", "master", "develop", "trunk") via the
new isConventionalDefaultName helper. The proxy doesn't have
label-resolution logic, so returning the default branch's HEAD for
any of these names is a reasonable approximation of what the v1.30.1
client intends (the default label's HEAD == default branch's HEAD).

The risk — a repo with a non-conventional default (e.g.,
"production") and a branch named "main" — returns HEAD instead of
the branch's commit, but this is the pre-Phase-18 behavior and the
v1.30.1 case is the common one.

Test coverage: TestGetMeta_ConventionalDefaultName_github and
TestGetMeta_ConventionalDefaultName_bitbucket both pin the live
googleapis scenario (default="master", commit="main") and assert
no GetCommit / /commits call is made.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Adds the testutil scaffolding for the new e2e test. RunBufGenerateWithPinnedLock
is the public wrapper; runBufGenerate is the private implementation that
writes buf.yaml + buf.gen.yaml + dummy.proto, runs `buf mod update` to
populate a real buf.lock, overwrites the lock's commit: line with the
pinned 32-char UUID via strings.Replace(..., 1), then runs `buf generate`
with a 120s timeout. The regex/extractor for the commit: line is duplicated
inline (Option A from the plan) because extractCommitFromLock lives in
package e2e and cannot be imported from the testutil package.

The buf.gen.yaml pins the remote plugin buf.build/protocolbuffers/go:v1.28.1
with out: gen/go relative to the workspace; the v1 buf.gen.yaml format is
understood by both v1.30.1 and v1.69.0.

Existing Phase 19 helpers (RunBufModUpdate, RunBufDepUpdate,
RunBufModUpdateWithRef, RunBufDepUpdateWithRef, runBufUpdate) are unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>
The new e2e test exercises a scenario distinct from the Phase 19
ref-honoring tests: the proxy must be able to serve a 'buf generate'
request when buf.lock already pins a valid (but non-HEAD) commit.

The test is a matrix over AvailableBufVersions, so it exercises both
BufV130 (v1alpha1 read-path) and BufV169 (v1beta1 read-path). It starts
a fresh proxy per version, derives the expected UUID from
'git ls-remote https://github.com/googleapis/googleapis
refs/tags/common-protos-1_3_1' via commitUUIDForTest (reused from
e2e/ref_test.go), calls the new testutil.RunBufGenerateWithPinnedLock
helper, and asserts:

  - exit 0 from `buf generate`
  - at least one generated gen/go/google/type/*.pb.go file is non-empty
  - the file's content contains 'package google.type' (catches a
    regression where the plugin runs but produces wrong content)

Skips cleanly (t.Skipf) when EASYP_GH_TOKEN is unset, so a token-less
`go test ./e2e/ -run TestGenerateWithPinnedBufLock -count=1` exits 0
with SKIP lines, not FAIL.

Co-Authored-By: Claude <noreply@anthropic.com>
Phase 21 deliverables complete:
- TestGenerateWithPinnedBufLock matrix e2e test (e2e/generate_test.go)
- RunBufGenerateWithPinnedLock + runBufGenerate testutil helpers
  (e2e/testutil/server.go)

Both success criteria (SC-21-1 matrix; SC-21-2 v1+v2 coverage) met.
The proxy's pinned-lock read-path now has a real-server e2e
regression guard: any future change that breaks the
infoCache writeback, the post-restart probeCommitID path, or
the commitUUIDInverse prefix-match check is caught at CI time
when a token + cached buf binaries are present.

Co-Authored-By: Claude <noreply@anthropic.com>
Both cached buf versions (v1.30.1 and v1.69.0) reject the 'remote:'
field in buf.gen.yaml:
  - v1.69.0 fails to decode the YAML ('line 3: field remote not
    found'); the alpha-remote-generation API was removed.
  - v1.30.1 prints 'the remote field no longer works as the remote
    generation alpha has been deprecated' and exits 1.

The correct modern syntax is the 'plugin:' field, which both
versions understand. The migration guide is at
https://buf.build/docs/migration-guides/migrate-remote-generation-alpha/

This was discovered when running TestGenerateWithPinnedBufLock with
a real EASYP_GH_TOKEN. The plan's YAML template was based on the
deprecated alpha API.

Co-Authored-By: Claude <noreply@anthropic.com>
Live test run with EASYP_GH_TOKEN revealed two real findings:

1. The test's buf.gen.yaml used the deprecated `remote:` field (alpha-
   remote-generation API). Both cached buf versions reject it:
   - v1.69.0: 'field remote not found' (removed entirely)
   - v1.30.1: 'remote field no longer works... alpha has been deprecated'

   Fix committed in e0c79b1 (use `plugin:` instead). SUMMARY updated
   to record the fix as Deviation #2.

2. The v1.30.1 subtest caught a real proxy regression: the v1alpha1
   `DownloadManifestAndBlobs` read-path passes 32-char buf UUIDs
   directly to GitHub's /git/trees/<id> API which 404s. Phase 18's
   `commitUUIDInverse` is only applied on the v1beta1 path; the
   v1alpha1 path was never updated. Phase 19/20 e2e tests missed this
   because they only exercised v1beta1 `buf mod update`.

3. The v1.69.0 subtest is not actually testing the pinned-UUID path
   (the v1.69.0 client ignores buf.lock for `buf generate` and just
   asks the proxy for HEAD) and is hitting a persistent TLS handshake
   timeout to raw.githubusercontent.com.

STATE.md updated with the v1.30.1 finding and a proposed Phase 22
follow-up: apply `commitUUIDInverse` in the v1alpha1 Download handler
chain, then re-run `TestGenerateWithPinnedBufLock` to confirm.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…ling

Phase 21's live e2e test (TestGenerateWithPinnedBufLock) caught a real
proxy regression: the v1.30.1 v1alpha1 DownloadManifestAndBlobs
read-path passes 32-char buf UUIDs directly to GitHub's
/git/trees/<id> API which 404s. Phase 18's commitUUIDInverse is only
applied on the v1beta1 path; the v1alpha1 path was never updated.

Phase 22 scope:
1. Apply commitUUIDInverse in the v1alpha1 Download handler chain so
   32-char buf UUIDs resolve to git SHAs before hitting GitHub's tree
   API.
2. Re-run TestGenerateWithPinnedBufLock to confirm both subtests pass.
3. Broader verification that the v1 (v1alpha1) protocol path works
   end-to-end (buf mod update, the smoke test Phase 19 revealed was
   broken for v1.30.1, and buf generate).

Depends on Phase 21. Not yet planned.

Co-Authored-By: Claude <noreply@anthropic.com>
onokonem and others added 12 commits July 8, 2026 16:53
Plan 22-01 wires the v1alpha1 DownloadManifestAndBlobs handler to the
Phase 18 UUID-resolution machinery (CommitResolver interface +
resolveCommitForRead wrapper + isUUID branch in blobs.go) so 32-char
buf-issued UUIDs resolve to git SHAs before hitting GetFiles. TDD:
Task 1 writes the three failing unit tests (PR-22-1/2/3), Task 2
implements the fix and turns them GREEN. The Phase 21
TestGenerateWithPinnedBufLock/v1.30.1 e2e is the PR-22-4 gate.

Co-Authored-By: Claude <noreply@anthropic.com>
Three RED tests covering PR-22-1/2/3 for the v1alpha1
DownloadManifestAndBlobs handler:

- TestDownloadManifestAndBlobs_ResolveUUID (PR-22-1): warm-cache case
  where a pre-seeded commitMap UUID must resolve to the git SHA before
  GetFiles is called.
- TestDownloadManifestAndBlobs_ProbeFallback (PR-22-2): cold-cache case
  where the UUID must reach probeCommitID (sourceCalls > 0).
- TestDownloadManifestAndBlobs_ResolveError (PR-22-3): unresolvable UUID
  must return a Connect error with zero GetFiles calls (no HEAD fallback).

All three fail against the unmodified handler (RED).
Wire the v1alpha1 DownloadManifestAndBlobs handler to the Phase 18
commit-resolution ladder so that 32-char buf-issued UUID references
are resolved to their 40-char git SHAs before being passed to
provider.GetFiles.

Changes across three files:
- api.go: add CommitResolver interface (resolveCommitForRead method),
  commitResolver field on *api, and a.commitResolver = commitHandler
  assignment in NewWithConfig (safe single-threaded post-construction).
- commits.go: add (*commitServiceHandler).resolveCommitForRead — a thin
  wrapper mirroring the ServeDownload decision ladder (commitMap →
  resolveForeignCommitID → probeCommitID) and returning an error on
  total miss (no silent HEAD fallback). Does NOT reimplement any Phase
  18 helper; delegates to probeCommitID verbatim.
- blobs.go: insert isUUID(ref) + commitResolver.resolveCommitForRead
  branch before the a.repo.GetFiles call. Nil-guarded for
  defense-in-depth.

ServeDownload is intentionally NOT refactored (RESEARCH.md Open
Question 2 — deferred). All Phase 18 guard tests pass unchanged.
SUMMARY documenting the CommitResolver interface wiring fix:
- Three new RED→GREEN tests (PR-22-1/2/3) all passing
- Phase 18 guards (PR-22-5) unchanged and passing
- PR-22-4 e2e gate SKIPs cleanly without EASYP_GH_TOKEN
- Two deviations documented (worktree base correction, CodeOK removal)
@onokonem
onokonem force-pushed the buf-proto-update-3 branch from ab6a99c to e7242c5 Compare July 8, 2026 13:55
@onokonem
onokonem merged commit 2ec011e into main Jul 8, 2026
1 check passed
onokonem added a commit that referenced this pull request Jul 10, 2026
7 findings from the post-merge review of PR #39 (buf-proto-update-3 to
main, 6184+/68-) all verified against current source:

- F-1: isConventionalDefaultName carve-out silently resolves HEAD for
  repos with real non-default branches named main/master/develop/trunk.
  Recommendation: remove the carve-out entirely (the v1.30.1 case it
  was designed for is no longer reachable after Phase 18).
- F-2: Fix misleading error wrap at blobs.go:45 (says GetRepository,
  calls GetFiles).
- F-3: Run the v1alpha1 live e2e gate (PR-22-4) now that TLS issue is
  resolved by df02ff0.
- F-4: Extract duplicated isSHA and isConventionalDefaultName from both
  providers to internal/providers/content/.
- F-5: Fix dangling proto path in commits_helpers_test.go:375.
- F-6: Eliminate commitLineRE duplication between e2e/ref_test.go and
  testutil/server.go via ExtractCommitFromLock export.
- F-7: Guard post-construction commitResolver mutation with a
  startup-time panic helper.

Co-Authored-By: Claude <noreply@anthropic.com>
onokonem added a commit that referenced this pull request Jul 10, 2026
3 plans in 2 waves:
- 25-01: Remove isConventionalDefaultName carve-out (FIX-01) + extract isSHA to content package (FIX-04)
- 25-02: Fix error wrap string (FIX-02), fix dangling proto path (FIX-05), add initCommitResolver guard (FIX-07)
- 25-03: Extract commitLineRE to testutil (FIX-06); human-verify v1alpha1 e2e gate (FIX-03)

Co-Authored-By: Claude <noreply@anthropic.com>
onokonem added a commit that referenced this pull request Jul 10, 2026
onokonem added a commit that referenced this pull request Jul 10, 2026
* test(23-01): add e2e tests for branch-name and non-default-branch commit refs in buf.yaml deps

Phase 23 closes the ref-shape coverage gap from Phase 19 (which covered
tag refs). Two new v1.69.0 e2e tests in e2e/ref_test.go:

- TestRefRespected_BranchName_PinsBranchTip: buf.yaml dep with ':gh-pages'
  (a non-default, non-conventional branch on googleapis/googleapis) pins
  buf.lock to commitUUIDForTest(gitLsRemote(gh-pages tip)). Proves the
  repos.GetCommit fall-through in GetMeta resolves branch names to tips.

- TestRefRespected_NonDefaultBranchCommitSHA: buf.yaml dep with the raw
  40-char SHA of the gh-pages tip (a commit off the default branch
  master) pins buf.lock to commitUUIDForTest(sha). Proves the isSHA fast
  path stamps the SHA branch-agnostically. Also surfaces whether the buf
  CLI accepts a raw-SHA ref suffix.

Both tests: derive ground-truth SHA at runtime via gitLsRemote, sanity
assert gh-pages tip != master tip, v1.69.0-only (strict UUID contract),
token-gated (skip cleanly when EASYP_GH_TOKEN unset). Reuse Phase 19
helpers (gitLsRemote/isLowerHex/commitUUIDForTest/extractCommitFromLock)
and RunBufModUpdateWithRef. No production code or testutil changes.

New const branchRef = "gh-pages". File 270 -> 394 lines.

Co-Authored-By: Claude <noreply@anthropic.com>

* docs(23): phase 23 plan + research + summary

Two v1.69.0 e2e tests for buf.yaml dependency refs:
- branch-name ref (gh-pages) pins to branch tip
- raw 40-char SHA ref of a commit off the default branch honored

Closes ref-shape coverage gap from Phase 19 (tags). Test code shipped
in 8cf99fa; this commit lands the planning artifacts.

Co-Authored-By: Claude <noreply@anthropic.com>

* docs(phase-23): evolve STATE.md + PROJECT.md after phase completion

- STATE.md: phase 23 status, progress 12/13 phases (94%), roadmap
  evolution entry, session continuity updated to 2026-07-09
- PROJECT.md: Current State reflects phase 23 (branch-name + non-default-
  branch commit ref e2e tests); phase 22 demoted to prior

Co-Authored-By: Claude <noreply@anthropic.com>

* test(24-01): add RED confirming tests for buf cid ref defects

Two failing tests pinning the prod failure (grpc-ecosystem/grpc-gateway
pinned at e91b8a68...):

- TestServeGraph_BufCommitIDRefNotForwardedToUpstream: cid must not be
  forwarded to provider.GetMeta as a git SHA (422).
- TestServeGraph_InfoCacheMustNotServeWrongCommit: infoCache keyed by
  owner/module must not serve a different commit for a pinned cid.

Plus buildV1GetGraphRequestWithRef + recordingProvider helpers.

RED base for Phase 24 execution.

Co-Authored-By: Claude <noreply@anthropic.com>

* docs(phase-24): plan + research + roadmap entry + debug diagnosis

24-01-PLAN.md: cid->sha map + ServeGraph UUID branch + infoCache
cid-gating + ServeDownload cid->sha preference (PR-24-1..6).
24-RESEARCH.md + debug/buf-cid-ref-forwarded-to-upstream.md.
ROADMAP phase entry; STATE -> executing.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(24-01): resolve cid refs in ServeGraph; gate infoCache by cid

Prod buf generate failed for grpc-ecosystem/grpc-gateway pinned in
buf.lock at commit_id e91b8a68fe214081808d79f1a1a4f09e: ServeGraph
forwarded the 32-hex buf-issued cid to GitHub (422 -> 502), and
infoCache keyed by owner/module served main HEAD's content under the
pinned cid. Same class of bug Phase 18 fixed for ServeDownload's probe
path; ported the resolution into ServeGraph + tightened infoCache.

- cidSha map stores cid -> full git sha at every mint site (GetCommits,
  ServeGraph writeback, ServeDownload mint, registerResolvedAlias).
- ServeGraph UUID branch: isUUID(ref.ref) -> resolveUUIDRef (cidSha hit
  or commitUUIDInverse -> 28-hex prefix probe). Never forwards the raw
  cid to GetMeta. Empty/branch/tag/SHA refs flow the existing path.
- infoCache hit gated on cached.commitID == requested cid when the
  request pins a cid, so a HEAD entry is not served for a pinned cid.
- ServeDownload files-cache hit gated on cid match; fetch path prefers
  cidSha over the owner/module infoCache entry so a pinned cid fetches
  its own content, not HEAD's.
- recordingProvider.GetMeta now mirrors real provider short-SHA prefix
  resolution so the 28-hex prefix path is exercisable in unit tests.

Turns the two Phase 24 RED tests GREEN; full internal/connect suite
and go vet clean.

Co-Authored-By: Claude <noreply@anthropic.com>

* test(24-01): add short-circuit, cold-cache probe, and download coverage

Three new tests covering the ServeGraph + ServeDownload cid-resolution
paths added in Task 24-01-01:

- TestServeGraph_UUIDRefShortCircuitsFromCidShaMap: warm cidSha ⇒ the
  pinned-cid request is served entirely from cache (zero GetMeta calls).
- TestServeGraph_UUIDRefColdCache_ProbesWithInversePrefix: cold cidSha ⇒
  GetMeta is called with the 28-hex prefix (never the raw cid); cidSha
  is cached afterwards.
- TestServeDownload_PinnedCidNotServedFromWrongInfoCache: infoCache
  holds HEAD's entry; the pinned cid's files are served via cidSha, not
  HEAD's. Seeds the handler directly to reproduce the prod
  wrong-infoCache state without fighting the cache-hit gate.

Co-Authored-By: Claude <noreply@anthropic.com>

* test(24-01): add e2e gate proving pinned cid serves pinned commit, not HEAD

TestGeneratePinnedCommit_NotHEAD pins buf.lock to the cid of a known-old
googleapis tag (common-protos-1_3_1, whose SHA differs from master HEAD)
and asserts the proxy's server log contains the PINNED commit's real git
SHA after buf generate. That SHA only appears if the Phase 24
cid->sha resolution (cidSha hit or commitUUIDInverse 28-hex prefix probe)
actually fired — in the bug state (cid forwarded upstream for 422, or
infoCache serving HEAD) the pinned SHA never appears.

Catches both prod defects: ServeGraph forwarding the cid (generate fails
outright) and infoCache serving HEAD under the pinned cid (generate
"succeeds" but emits the wrong code).

Gated on EASYP_GH_TOKEN; skips cleanly without it.

Co-Authored-By: Claude <noreply@anthropic.com>

* docs(24-01): SUMMARY for resolve-buf-cid-ref-in-servegraph-honor-pinned-commit

Co-Authored-By: Claude <noreply@anthropic.com>

* docs(phase-24): update tracking after wave 1

* docs(24): add code review report

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(24-01): CR-01 + WR-02 inherit probeCommitID defenses in resolveUUIDRef

The cold-cache prefix-probe path in resolveUUIDRef bypassed every safety
mechanism probeCommitID was instrumented with: probeSem (concurrency cap
on upstream probes), missCache (negative cache for known-bogus ids),
probeTimeout (per-call bound on hangs), and isTransientErr classification
(keeping transient outages from permanently locking out a real cid). An
unauthenticated client flooding ServeGraph with distinct 32-hex cids
caused one unbounded upstream GetMeta per request.

Replicate all four defenses inside resolveUUIDRef (routing through
probeCommitID directly is structurally infeasible: probeCommitID fans
out across ALL configured sources, but ServeGraph has the specific
owner/module from the request). The per-call timeout is guarded on
probeTimeout > 0 so the zero-value "enhancements off" default preserves
the request-context-only behavior tests rely on.

New test TestServeGraph_UUIDRefColdCache_NegativeCachesMiss asserts the
negative cache suppresses repeat probes for an unknown cid within TTL.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(24-01): WR-01 symmetric infoCache cid-gate + cidPinned flag

The ServeGraph infoCache cid-gate only protected one direction: a
pinned-cid request was not served a HEAD-minted entry. The reverse was
unprotected — a single pinned-cid writeback poisoned every subsequent
HEAD/SHA/tag request on the same module until restart (the gate's
!isUUID(ref.ref) clause short-circuited to HIT for any non-cid request).

commitInfoCache.commitID is always a cid (the output of commitUUID), so
isUUID(cached.commitID) cannot distinguish a pinned entry from a
HEAD-minted one. Added a cidPinned bool set at every mint site
(ServeHTTP, ServeGraph, registerResolvedAlias) recording whether the
entry was produced by a cid-pinned request. The gate is now symmetric:
non-UUID requests only hit non-pinned entries; UUID requests only hit
exact-cid matches.

recordingProvider now models HEAD (empty commit arg) via an explicit
head field and guards the prefix-match loop against empty commit
(HasPrefix(x, "") matches everything).

New test TestServeGraph_PinnedCidDoesNotPoisonHeadRequest asserts a
HEAD request after a pinned-cid writeback re-resolves instead of
serving the pinned cid's content.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(24-01): WR-03 ServeDownload fetch writes back infoCache + filesMap

The ServeDownload fetch path only wrote cidSha[cid]=meta.Commit after
resolving a pinned cid. It left infoCache holding the stale pre-resolution
entry and never populated filesMap[cid], so every subsequent identical
pinned-cid Download re-ran cidShaLookup (hit) → GetMeta → GetFiles →
re-compute digest on every request.

Now writes the full resolution (commitMap, cidSha, infoCache, filesMap)
after a successful fetch, mirroring the ServeGraph and ServeHTTP
writebacks. The infoCache entry carries cidPinned=isUUID(commitID) so
the symmetric WR-01 gate stays consistent across handlers.

New test TestServeDownload_PinnedCidRepeatHitsFilesCache asserts the
second identical pinned-cid Download makes zero new GetMeta/GetFiles
calls.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(24-01): WR-04 strengthen e2e not-HEAD assertion

TestGeneratePinnedCommit_NotHEAD asserted every generated file contained
`package google.type`, but that marker exists at both the pinned tag
(common-protos-1_3_1) and HEAD, so it cannot detect a bug where the proxy
served HEAD's content under the pinned cid. The only load-bearing check
was a bare substring match on the pinned SHA anywhere in the server log,
which proves the SHA was logged but not that its content was served.

Replaced with a structured assertion: the server log must contain the
pinned SHA as the value of a `commit=` attribute on a line tagged with a
serving-decision branch (uuid_ref_resolved, info_cache_writeback,
files_cache_hit, digest_b5_wrap, digest_b4_keep, or
commit_id_probe_hit). Each of those branches only runs after the proxy
actually fetched and processed the pinned commit's content, so the
assertion now proves the pinned commit was served, not merely referenced.

The `package google.type` content check was dropped as non-load-bearing;
the non-empty file check is retained as a sanity gate.

Co-Authored-By: Claude <noreply@anthropic.com>

* docs(24-01): IN-01 document 28-hex uniqueness invariant at call sites

commitUUIDInverse recovers only the first 14 bytes (28 hex) of the
original SHA. The prefix-match validation in resolveUUIDRef and
probeCommitID trusts that at most one commit in the upstream repo starts
with those 28 hex chars (a 2^112 collision space). The assumption was
implicit and undocumented at the call sites.

The resolveUUIDRef site was annotated during the CR-01 fix; this commit
strengthens the probeCommitID site to explicitly name the 2^112
uniqueness invariant (IN-01) and the HasPrefix verification that closes
the wrong-source alias risk. Documentation hardening only — no behavior
change.

Co-Authored-By: Claude <noreply@anthropic.com>

* docs(24): mark all 6 REVIEW.md findings as fixed

CR-01, WR-01, WR-02, WR-03, WR-04, IN-01 all resolved. Frontmatter
status -> all_fixed. Each finding now carries a Status line citing the
fix commit and the test that covers it.

Co-Authored-By: Claude <noreply@anthropic.com>

* test(24): add goal verification report (human_needed — live-token e2e)

* test(24): persist human verification items as UAT

* test(24): record live e2e blocked-by-env finding with baseline control

* feat(github): retry transient upstream errors in HTTP transport

The GitHub provider fans out GetTree + per-file DownloadContents GETs
(api.github.com + raw.githubusercontent.com). A single transient TLS
handshake timeout / EOF / connection reset on any one request fails the
whole batch — observed as intermittent "server hosted at that remote
is unavailable" / 502 / digest-verification failures under flaky
network egress.

Wrap the provider http.Client's transport with a bounded retry layer
(retryTransport):
- retries idempotent GET/HEAD/OPTIONS only
- retries transient net errors (net.Error timeout, EOF, reset) and
  HTTP 429/500/502/503/504
- honors Retry-After, else exponential backoff + jitter (capped 8s),
  max 4 attempts
- never retries caller context.Canceled (intentional)
- drains partial response bodies between attempts

Unit coverage: retry-then-succeed, 5xx-then-succeed, give-up-after-max,
no-retry-on-cancel, no-retry-non-idempotent, Retry-After parse, status
classification.

Co-Authored-By: Claude <noreply@anthropic.com>

* docs(25): research PR #39 post-merge review findings

7 findings from the post-merge review of PR #39 (buf-proto-update-3 to
main, 6184+/68-) all verified against current source:

- F-1: isConventionalDefaultName carve-out silently resolves HEAD for
  repos with real non-default branches named main/master/develop/trunk.
  Recommendation: remove the carve-out entirely (the v1.30.1 case it
  was designed for is no longer reachable after Phase 18).
- F-2: Fix misleading error wrap at blobs.go:45 (says GetRepository,
  calls GetFiles).
- F-3: Run the v1alpha1 live e2e gate (PR-22-4) now that TLS issue is
  resolved by df02ff0.
- F-4: Extract duplicated isSHA and isConventionalDefaultName from both
  providers to internal/providers/content/.
- F-5: Fix dangling proto path in commits_helpers_test.go:375.
- F-6: Eliminate commitLineRE duplication between e2e/ref_test.go and
  testutil/server.go via ExtractCommitFromLock export.
- F-7: Guard post-construction commitResolver mutation with a
  startup-time panic helper.

Co-Authored-By: Claude <noreply@anthropic.com>

* docs(25): create phase plan for PR #39 post-merge review findings

3 plans in 2 waves:
- 25-01: Remove isConventionalDefaultName carve-out (FIX-01) + extract isSHA to content package (FIX-04)
- 25-02: Fix error wrap string (FIX-02), fix dangling proto path (FIX-05), add initCommitResolver guard (FIX-07)
- 25-03: Extract commitLineRE to testutil (FIX-06); human-verify v1alpha1 e2e gate (FIX-03)

Co-Authored-By: Claude <noreply@anthropic.com>

* docs(25): create phase plan

* fix(25-01): remove isConventionalDefaultName carve-out, extract isSHA to content package

Remove the isConventionalDefaultName carve-out from both providers
(github/getrepo.go and bitbucket/getrepo.go) that silently returned HEAD
for repos with real branches named main/master/develop/trunk not set as
the default. The v1.30.1 label_name=3 case it was designed for is no
longer reachable (Phase 18 changed parseResourceRefName to read field 4
only).

Extract duplicated isSHA helper to internal/providers/content/helpers.go
as exported content.IsSHA and content.IsConventionalDefaultName.
Add helpers_test.go with 8 IsSHA test cases and 5 IsConventionalDefaultName
test cases.

Remove TestGetMeta_ConventionalDefaultName tests from both providers.
Update TestGetMeta_DefaultBranchName doc comments.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(25-02): fix error wrap string, proto path comment, add initCommitResolver

FIX-02: Fix misleading error wrap in blobs.go:45 — says 'a.repo.GetFiles'
not 'a.repo.GetRepository' (the actual method called).

FIX-05: Fix dangling proto path in commits_helpers_test.go:375 —
replace non-existent v1beta1/resource.proto with general 'buf BSR Name
message' description.

FIX-07: Add initCommitResolver helper to api.go with panic-guard against
nil, replacing the direct a.commitResolver = commitHandler assignment.

Co-Authored-By: Claude <noreply@anthropic.com>

* refactor(25-03): extract commitLineRE and ExtractCommitFromLock to testutil

FIX-06: Eliminate duplicated commitLineRE regex between e2e/ref_test.go
and e2e/testutil/server.go. Export CommitLineRE and ExtractCommitFromLock
from testutil, remove local definitions from ref_test.go, update all 7
call sites to use testutil.ExtractCommitFromLock.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(25-01): restore isConventionalDefaultName carve-out via shared content helper

The v1alpha1 ResolveService handler (modulepins.go GetModulePins) is
NOT gated by parseResourceRefName and still passes label_name='main'
(proto field 3) as commit='main' to GetMeta. The carve-out condition
was restored using the shared content.IsConventionalDefaultName helper
extracted in the previous commit. Verified by e2e: all 6 TestRefRespected
subtests pass against real GitHub with EASYP_GH_TOKEN.

Co-Authored-By: Claude <noreply@anthropic.com>

* docs(25-01): complete remove carve-out and extract helpers plan summary
docs(25-02): complete mechanical code-quality fixes plan summary
docs(25-03): complete commitLineRE extraction and e2e verification plan summary

* docs(25): update STATE.md for Phase 25 completion — all 7 PR #39 findings addressed

* docs(25): ship phase 25 — PR #40

---------

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant